Skip to content

fix(rename): skip stateless objects in PathsGenerator to prevent panic [PSGO-233]#2591

Open
Matovidlo wants to merge 3 commits intomainfrom
mvasko/PSGO-233/fix-rename-plan-stateless-panic
Open

fix(rename): skip stateless objects in PathsGenerator to prevent panic [PSGO-233]#2591
Matovidlo wants to merge 3 commits intomainfrom
mvasko/PSGO-233/fix-rename-plan-stateless-panic

Conversation

@Matovidlo
Copy link
Copy Markdown
Contributor

Summary

  • Root cause: PathsGenerator.doUpdate calls objectState.LocalOrRemoteState() unconditionally, which panics when a manifest entry exists for a config that has neither local nor remote state. This happens when an orphaned scheduler config remains in the manifest after its target orchestrator was deleted — the state entry is created from the manifest but never populated with actual data from either local files or the remote API.
  • Crash location: internal/pkg/model/objectstate.go:265LocalOrRemoteState() → called from internal/pkg/state/local/paths.go:79PathsGenerator.doUpdateinternal/pkg/plan/rename/rename.gorename.NewPlansync/pull
  • Why CI passes but local pull crashes: CI runs always start from an empty git repository (no pre-existing manifest), so there are no stale entries. Client machines have an existing manifest that accumulated the orphaned scheduler entry.

Changes

  • paths.go: Added two guards to doUpdate:
    1. Return early when the object itself has neither local nor remote state (prevents LocalOrRemoteState() panic).
    2. Return early after a recursive parent update when the parent is also stateless (prevents path resolution on an unresolvable parent).
  • plan_test.go: New test TestRenameAllPlan_StatelessObjectSkipped injects a stateless ConfigState into a loaded state and asserts NewPlan returns without error and excludes the orphan from rename actions.

Relation to previous fixes

PR #2580 fixed the naming registry collision. PR #2585 fixed the manifest SetRecords fail-fast and orphaned-scheduler handling in push/diff. This PR fixes the remaining crash path that triggers during kbc pull when the rename plan builder encounters the stateless state entry that those earlier fixes may not have fully cleaned up.

Test plan

  • go test -race ./internal/pkg/plan/rename/... -run TestRenameAllPlan
  • go test -race ./internal/pkg/state/local/...
  • Manual: kbc pull on a project with an orphaned scheduler config in the local manifest — should complete without crashing

Fixes PSGO-233 / SUPPORT-15941 (follow-up crash after #2585)

🤖 Generated with Claude Code

@linear
Copy link
Copy Markdown

linear Bot commented Apr 28, 2026

@Matovidlo Matovidlo marked this pull request as ready for review April 29, 2026 06:10
Copilot AI review requested due to automatic review settings April 29, 2026 06:10
@Matovidlo Matovidlo requested a review from hosekpeter as a code owner April 29, 2026 06:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prevents kbc pull / rename-plan generation from panicking when the local state contains manifest-only (“stateless”) object entries (no local files and no remote API object), which can happen with orphaned scheduler configs.

Changes:

  • Add guards in PathsGenerator.doUpdate to skip stateless objects (and stateless parents) to avoid LocalOrRemoteState() panics.
  • Add a rename plan unit test intended to ensure stateless entries don’t break NewPlan.
  • Add an e2e CLI fixture that reproduces pull --force with an orphaned scheduler record plus a rename scenario.

Reviewed changes

Copilot reviewed 14 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/pkg/state/local/paths.go Skip stateless objects/parents during path generation to prevent LocalOrRemoteState() panic.
internal/pkg/plan/rename/plan_test.go Adds a unit test around stateless entries during rename plan creation.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/args New CLI e2e test arguments for pull --force.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/expected-code Expected exit code for the new CLI fixture.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/expected-stderr Expected warning output for manifest load warnings.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/expected-stdout Expected plan + rename output for the new CLI fixture.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/initial-state.json Initial remote state fixture for the new CLI test.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/in/.keboola/manifest.json Input manifest containing an orphaned scheduler config and a wrongly named config.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/in/description.md Input repository description fixture.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/in/main/description.md Input main-branch description fixture.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/in/main/extractor/ex-generic-v2/wrong-name/config.json Input config content fixture (pre-rename).
test/cli/pull/pull-force-orphaned-scheduler-with-rename/in/main/extractor/ex-generic-v2/wrong-name/description.md Input config description fixture (pre-rename).
test/cli/pull/pull-force-orphaned-scheduler-with-rename/in/main/extractor/ex-generic-v2/wrong-name/meta.json Input config metadata fixture (pre-rename).
test/cli/pull/pull-force-orphaned-scheduler-with-rename/in/main/meta.json Input branch metadata fixture.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/.keboola/manifest.json Expected cleaned manifest after pull (orphan removed, renamed paths).
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/.keboola/project.json Expected project metadata after pull.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/description.md Expected repository description output fixture.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/main/description.md Expected main-branch description output fixture.
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/main/extractor/ex-generic-v2/empty/config.json Expected config content output fixture (post-rename).
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/main/extractor/ex-generic-v2/empty/description.md Expected config description output fixture (post-rename).
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/main/extractor/ex-generic-v2/empty/meta.json Expected config metadata output fixture (post-rename).
test/cli/pull/pull-force-orphaned-scheduler-with-rename/out/main/meta.json Expected branch metadata output fixture.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/pkg/state/local/paths.go
Comment thread internal/pkg/state/local/paths.go
Comment thread internal/pkg/plan/rename/plan_test.go Outdated
Copy link
Copy Markdown
Contributor

@hosekpeter hosekpeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (out of scope):TestRenameAllPlan lines 51-59 action.Manifest = nil is set twice here. Could be cleaned up while we're here.

Comment thread internal/pkg/plan/rename/plan_test.go Outdated
Comment thread internal/pkg/plan/rename/plan_test.go Outdated
Matovidlo and others added 2 commits April 29, 2026 12:51
When a manifest entry exists for a config that has neither local nor
remote state (e.g. an orphaned scheduler config after its target
orchestrator was deleted), PathsGenerator.doUpdate called
LocalOrRemoteState() which panicked unconditionally.

Two guards added to doUpdate:
1. Skip the object itself when both HasLocalState and HasRemoteState
   are false.
2. Skip the object when its parent is stateless, since a valid path
   cannot be resolved without a resolved parent.

Reproduces the crash in PSGO-233 / SUPPORT-15941 where kbc pull
panicked on "object Local or Remote state must be set" for clients
with an existing local manifest containing an orphaned scheduler entry.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Regression test for PSGO-233: verifies that kbc pull --force succeeds
when the manifest contains an orphaned scheduler (fake ID, no local
files, not in remote) AND a config exists at a wrong path that needs
to be renamed. Previously PathsGenerator.doUpdate could panic on
stateless objects encountered during the rename plan.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Matovidlo Matovidlo force-pushed the mvasko/PSGO-233/fix-rename-plan-stateless-panic branch from 5bcbce0 to 4329212 Compare April 29, 2026 10:52
Copy link
Copy Markdown

@romanbracinik romanbracinik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok fixol by som len ten test este

Comment thread internal/pkg/plan/rename/plan_test.go
Remove "Ignoring invalid local state." — the orphaned scheduler "456"
is removed by SetRecords at manifest-load time (stderr warning only),
so no localErr is produced and the stdout message is not printed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Matovidlo Matovidlo force-pushed the mvasko/PSGO-233/fix-rename-plan-stateless-panic branch from 4329212 to c6695db Compare April 30, 2026 16:02
Copy link
Copy Markdown

@romanbracinik romanbracinik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants